[EXP-23262]: S7: support subrepo cloning via GH_USER / GH_TOKEN by oleksandrhleba · Pull Request #92 · readdle/system7

oleksandrhleba · 2026-06-30T16:09:03Z

Ticket link

Summary

Adds native support in s7 for authenticating GitHub subrepo network operations over HTTPS with a Personal Access Token, instead of relying on an SSH key.

When S7_GIT_USER + S7_GIT_TOKEN (or the GH_USER / GH_TOKEN fallback) are present, s7 makes git rewrite SSH github.com URLs to HTTPS and supplies the PAT as an HTTP Authorization header. When they're absent, behavior is unchanged — SSH as before.

How it works

All git invocations funnel through +[GitRepository runGitWithArguments:…]. When both credentials are present, s7 injects three GIT_CONFIG_* entries into the child git process's environment:

url.https://github.com/.insteadOf = git@github.com:
url.https://github.com/.insteadOf = ssh://git@github.com/
http.https://github.com/.extraheader = Authorization: Basic base64(user:token)

insteadOf moves the subrepo URL onto HTTPS transport; the github.com‑scoped extraheader authenticates it — the same approach actions/checkout uses. This is git ≥ 2.31's env-based config; no git config file is touched.

Security properties

Token never in a URL — it rides in an HTTP header, so it can't leak through git error messages, GIT_TRACE_CURL, or remote.origin.url. base64 (not percent-encoded creds in a URL) also means arbitrary token bytes are safe.
Token never in argv — delivered via environment, not -c, so it can't appear in ps / /proc / crash reports.
Token never on disk — .git/config and .s7substate keep the original SSH URL; the rewrite is resolution-time only.
Header scoped to github.com — never attached to a redirect or other host.

Behavior details

No-op off the token path. With no credentials, task.environment is left untouched, so dev machines and SSH users are completely unaffected.
Composes with inherited config. New entries append past any existing GIT_CONFIG_COUNT, so a nested s7 (which inherits the parent's injected config) or any other tool's GIT_CONFIG_* is never clobbered.
Diagnostic. On the token path, s7 prints s7: subrepo network auth: HTTPS via token to stderr (once) so CI auth failures are easy to spot.

Testing

Unit tests (gitGitHubTokenAuthTests.m): the config builder produces the correct entries, appends past an existing count, and the raw token never appears outside the base64 header. Builds clean under -Werror.
Manually verified end-to-end against a real PAT on a clone of rd2 (SSH disabled via GIT_SSH_COMMAND=false to prove the HTTPS path).

Scope & follow-ups

No automated CI smoke against a live PAT yet — recommend adding a gated git ls-remote check.

pastey

I'm still not sure if we need GH_USER/GH_TOKEN in the s7 code. What are your thoughts?

pastey · 2026-07-02T08:04:16Z

+// GIT_CONFIG_COUNT) doesn't clobber it.
+ (NSDictionary<NSString *, NSString *> *)gitHubTokenConfigEnvironmentForUser:(nullable NSString *)user
+                                                                        token:(nullable NSString *)token
+                                                          existingConfigCount:(NSInteger)existingConfigCount


instead of passing existingConfigCount argument, I would directly access the

NSDictionary<NSString *, NSString *> *const env = NSProcessInfo.processInfo.environment; const NSInteger existingConfigCount = MAX(0, [env[@"GIT_CONFIG_COUNT"] integerValue]);

here. This way all the logic managing GIT_CONFIG_COUNT would be in a single place. Now it's spread out by two methods.
Plus NSInteger seems odd. Why not unsigned?

Valid point. Fixed with one small deviation: reading NSProcessInfo inside the builder would kill the tests, so instead the builder takes the environment as a parameter.

oleksandrhleba · 2026-07-06T08:40:41Z

I'm still not sure if we need GH_USER/GH_TOKEN in the s7 code. What are your thoughts?

I've added those since they're used on Jenkins rn. And as far as I understand - Jenkins will use one PAT for all. So with current state of Jenkins we will have GH_* and S7_GIT_* values duplicated.

S7_GIT_* are working in parallel and have higher priority. So in case if we will need to use separate token for S7 - we could.

TBH I don't see the point in having duplicated values for two keys. But I do see actual case when we would like to have S7 specific credentials.

In case if on system level we actually will retire GH_* env variable usage for all the project repos - then I'm fine to delete it :)

pastey

Thank you for the changes. Looks almost perfect now 😍
Have few more suggestions 🙏

As for the GH_USER / S7_ – let it be. Duplicating the env variable in Jenkins configs will be too much of a pain indeed.

pastey · 2026-07-06T14:39:19Z

+    }
+
+    NSMutableDictionary<NSString *, NSString *> *const result = [NSMutableDictionary new];
+    __block NSUInteger nextConfigPairIndex = (NSUInteger)MAX(0, [processEnvironment[@"GIT_CONFIG_COUNT"] integerValue]);


nit:

Suggested change

__block NSUInteger nextConfigPairIndex = (NSUInteger)MAX(0, [processEnvironment[@"GIT_CONFIG_COUNT"] integerValue]);

__block NSUInteger nextConfigPairIndex = MAX(0, [processEnvironment[@"GIT_CONFIG_COUNT"] unsignedIntegerValue]);

processEnvironment[@"GIT_CONFIG_COUNT"] returns string, not NSNumber, so I won't be able to use unsignedIntegerValue here.

My bad. Thanks 👍

pastey · 2026-07-07T06:28:00Z

+        if (configEnvironment.count > 0) {
+            NSMutableDictionary<NSString *, NSString *> *const merged = [env mutableCopy];
+            [merged addEntriesFromDictionary:configEnvironment];
+            taskEnvironment = [merged copy];


I would suggest to put all the merging logic into the gitHubTokenConfigEnvironmentForUser itself.
Now the return value of the gitHubTokenConfigEnvironmentForUser is inconsistent: at one hand, it contains just the values we want to add, but the GIT_CONFIG_COUNT is the sum of our custom values and the keys from the original environment. If we try to use gitHubTokenConfigEnvironmentForUser as it's written now in some other place, we'd have to add the same merge there too.

Plus, I would suggest to remove the call for copy.
- taskEnvironment = [merged copy];
+ taskEnvironment = merged;

I don't think we have some unscrupulous people casting NSDictionary to NSMutableDictionary around here.

pastey · 2026-07-07T06:30:24Z

+// keeps the original SSH URL, so the token never lands on disk either.
+ (nullable NSDictionary<NSString *, NSString *> *)gitHubTokenAuthTaskEnvironment {
+    static dispatch_once_t onceToken;
+    static NSDictionary<NSString *, NSString *> *taskEnvironment;


I'm always scared by uninitialised variable. Even the static ones.

Suggested change

static NSDictionary<NSString *, NSString *> *taskEnvironment;

static NSDictionary<NSString *, NSString *> *taskEnvironment = nil;

pastey

Thank you!

Added PAT support for subrepo cloning

e595cbd

oleksandrhleba requested review from a-olkhovskyi and pastey and removed request for a-olkhovskyi June 30, 2026 16:09

pastey requested changes Jul 2, 2026

View reviewed changes

Adjusted based on code review

ea7348e

oleksandrhleba requested a review from pastey July 6, 2026 14:20

pastey requested changes Jul 7, 2026

View reviewed changes

Updates after code review

027fc05

oleksandrhleba requested a review from pastey July 7, 2026 14:03

pastey approved these changes Jul 7, 2026

View reviewed changes

oleksandrhleba merged commit a394423 into main Jul 7, 2026
3 checks passed

oleksandrhleba deleted the task/EXP-23262-github-token-auth branch July 7, 2026 20:04

	__block NSUInteger nextConfigPairIndex = (NSUInteger)MAX(0, [processEnvironment[@"GIT_CONFIG_COUNT"] integerValue]);
	__block NSUInteger nextConfigPairIndex = MAX(0, [processEnvironment[@"GIT_CONFIG_COUNT"] unsignedIntegerValue]);

	static NSDictionary<NSString , NSString > *taskEnvironment;
	static NSDictionary<NSString , NSString > *taskEnvironment = nil;

Uh oh!

Conversation

oleksandrhleba commented Jun 30, 2026 • edited by atlassian Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pastey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pastey Jul 2, 2026

Choose a reason for hiding this comment

Uh oh!

oleksandrhleba Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

oleksandrhleba commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pastey left a comment

Choose a reason for hiding this comment

Uh oh!

pastey Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

oleksandrhleba Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pastey Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

pastey Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

pastey Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

pastey Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

pastey left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

oleksandrhleba commented Jun 30, 2026 •

edited by atlassian Bot

Loading

oleksandrhleba commented Jul 6, 2026 •

edited

Loading

oleksandrhleba Jul 7, 2026 •

edited

Loading